Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor Typos #2100

Closed
wants to merge 7 commits into from
Closed

Minor Typos #2100

wants to merge 7 commits into from

Conversation

knightwayne
Copy link

Fixed Minor Typos, should commit made for previous pull requests! #2099

@EwoutH EwoutH requested a review from rht April 2, 2024 14:21
@EwoutH EwoutH added the docs Release notes label label Apr 2, 2024
@EwoutH
Copy link
Member

EwoutH commented Apr 17, 2024

Thanks for the PR! While the typo fixes are good, I think this breaks our links the ReadTheDocs.

CC @rht

knightwayne
knightwayne commented Apr 23, 2024
View reviewed changes
Copy link
Author

@knightwayne knightwayne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed links from GitHub to ReadtheDocs

@knightwayne
Copy link
Author

I think this also supercedes the previous PR #2099 .
I think the last changes should work, let me know if I need to modify something esle here. Thanks!
@rht

@knightwayne knightwayne mentioned this pull request Apr 23, 2024
Closed
rht
rht reviewed Apr 23, 2024
View reviewed changes
docs/overview.md
@@ -18,8 +18,8 @@ Most models consist of one class to represent the model itself; one class (or mo

- `mesa.Model`
- `mesa.Agent`
- [mesa.time](apis/time.md)
- [mesa.space](apis/space.md)
- [mesa.time](https://mesa.readthedocs.io/en/stable/apis/time.html)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch name (stable) can't be hardcoded, because with this, new changes from the PRs can no longer be checked.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there are other versions for the rtd, like stable and latest. But I am not 100% sure what my approach should be here?
Should defining a variable which captures the current version of readthedocs and use that + [the correct address] to redirect the links or something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to figure out RTD and Sphinx configuration such that apis/space.md gets translated to the correct URL path upon build.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I thought it needs to be configured using the folder structure rather than hardcoding the paths, because it looked like auto-documentation generator tool. Will look into it, and try to solve the issue. Thanks!

@tpike3
Copy link
Member

tpike3 commented May 5, 2024

@knightwayne Thank you for doing this. Anything we can do to help you get this merged? Any questions?

@tpike3 tpike3 added the Sprints! A task that might be good to tackle during sprints! label May 18, 2024
@quaquel
Copy link
Member

quaquel commented Oct 21, 2024

closed via #2392 and various earlier PRs.

Thanks again for pointing out these issues and presenting solutions.

@quaquel quaquel closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rht rht rht left review comments

Assignees
No one assigned
Labels
docs Release notes label Sprints! A task that might be good to tackle during sprints!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@knightwayne @EwoutH @tpike3 @quaquel @rht